src/NeuralNetworks/Layers/Graph/GraphSAGELayer.cs (1)

43-46: Documentation still claims LSTM aggregator which is not implemented

The remark:

/// AGGREGATE is an aggregation function (mean, max, LSTM), and σ is an activation function.

doesn’t match SAGEAggregatorType (Mean, MaxPool, Sum) or the implementation in AggregateNeighbors. Either add an LSTM aggregator or, more realistically for this PR, update the docs to reflect the actual options (Mean, MaxPool, Sum) so users aren’t misled.

src/NeuralNetworks/Layers/Graph/GraphConvolutionalLayer.cs (1)

26-37: Interface implementation and feature/adjacency accessors look good

Implementing IGraphConvolutionLayer<T> here and exposing InputFeatures / OutputFeatures plus GetAdjacencyMatrix() is a clean way to standardize graph-layer behavior; it should make higher-level utilities and tests much easier.

Two optional improvements you might consider later:

• In Forward / SetAdjacencyMatrix, use InputFeatures / OutputFeatures to assert shape consistency (e.g., that input.Shape[2] == InputFeatures and adjacency dimensions match numNodes), so misconfigured graphs fail fast with a clear message instead of surfacing as lower-level tensor/matrix errors.
• If you expect adapter/wrapper layers (e.g., LoRA) to interrogate feature counts, ensure they delegate these properties correctly to avoid losing this metadata.

As-is, the added surface looks consistent and non-breaking.

Also applies to: 163-172, 196-205, 295-315

src/NeuralNetworks/Layers/Graph/IGraphConvolutionLayer.cs (1)

3-25: Interface contract is good, but formal relationship to ILayer<T> should be clarified

The IGraphConvolutionLayer<T> surface (adjacency management + InputFeatures/OutputFeatures) is sensible, but the relationship to ILayer<T> creates a design inconsistency:

• The XML docs state that this "extends the base layer interface", yet the interface does not formally inherit from ILayer<T>.
• In practice, all graph layer implementations inherit from LayerBase<T> (which implements ILayer<T>) and implement IGraphConvolutionLayer<T>, so runtime instances are both types. However, the type system doesn't express this relationship.
• Code that needs both graph and standard layer semantics must either cast or maintain references to both unrelated interfaces, which complicates usage and adapter implementations (e.g., LoRA wrappers).

Recommended: Update the interface declaration to public interface IGraphConvolutionLayer<T> : ILayer<T> (and adjust implementations accordingly). This formalizes what the implementations already guarantee and simplifies code that requires both capabilities.

If changing the inheritance is undesirable, clarify the documentation to remove the suggestion of an inheritance relationship.

src/NeuralNetworks/Layers/Graph/GraphAttentionLayer.cs (1)

38-40: _dropoutRate is never used in the attention computation

_dropoutRate is accepted and stored, but there’s no dropout applied to the attention coefficients or transformed features in Forward. That’s slightly misleading given the XML docs and typical GAT behavior.

Consider either:

• Implementing dropout on the attention coefficients (e.g., after softmax and before aggregation), or
• Removing the parameter/field and docs for now to avoid implying a feature that isn’t there.

Also applies to: 259-324

src/NeuralNetworks/Layers/Graph/GraphSAGELayer.cs (1)

224-309: Backward path ignores neighbor aggregation and L2 normalization gradients

AggregateNeighbors supports Mean/MaxPool/Sum, and Forward optionally applies L2Normalize, but Backward:

• Computes gradients for _selfWeights, _neighborWeights, and _bias using _lastInput and _lastAggregated, yet
• Only propagates input gradients through _selfWeights, explicitly noting that backprop through aggregation is “simplified”, and
• Doesn’t account for the L2 normalization step when _normalize is true.

This is fine as an initial implementation, but it means:

• Upstream layers don’t receive gradients due to neighbor contributions, and
• The effect of normalization isn’t reflected in gradients.

If you plan to train with this layer, consider a follow‑up that:

• Backprops through AggregateNeighbors (similar to how GIN handles its aggregation), and
• Either backprops through L2Normalize or documents that normalization is treated as non‑differentiable.

Also applies to: 423-495

tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/GraphLayerTests.cs (1)

13-471: Forward and interface tests are solid; consider adding minimal backward/update smoke tests

The suite does a good job validating constructors, adjacency preconditions, output shapes, and IGraphConvolutionLayer<T> compliance across GCN, GAT, GraphSAGE, and GIN.

Given these layers are intended for training, it would be valuable to add at least one small test per layer that:

• Calls Forward, then Backward with a simple gradient tensor, and then UpdateParameters.
• Asserts that parameter vectors (GetParameters()) actually change.

That kind of smoke test would quickly catch issues like missing gradients or parameters never being updated (e.g., for attention weights).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/LoRA/DefaultLoRAConfiguration.cs (1)

206-220: Docs now match behavior for graph convolution; consider generalizing wording for all graph layers

The remarks correctly call out GraphConvolutionalLayer as excluded from LoRA, matching the new runtime behavior. Given the new GNN layers (GraphAttentionLayer, GraphSAGELayer, GraphIsomorphismLayer) will also currently pass through DefaultLoRAConfiguration unchanged, you may want to slightly generalize this note (e.g., “graph convolution layers implementing IGraphConvolutionLayer require a specialized adapter”) so the documentation stays accurate as additional graph layers are added.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/NeuralNetworks/Layers/Graph/GraphAttentionLayer.cs (1)

381-398: Consider making multi-head aggregation configurable.

The implementation averages multi-head outputs (line 395). While this is valid for final GAT layers, intermediate layers typically concatenate heads for greater expressiveness. Consider adding a constructor parameter to choose between averaging and concatenation.

Example:

+ public enum MultiHeadAggregation { Average, Concatenate }
+ private readonly MultiHeadAggregation _aggregation;

  public GraphAttentionLayer(
      int inputFeatures,
      int outputFeatures,
      int numHeads = 1,
      double alpha = 0.2,
      double dropoutRate = 0.0,
+     MultiHeadAggregation aggregation = MultiHeadAggregation.Average,
      IActivationFunction<T>? activationFunction = null)

Note: Concatenation would require outputFeatures to be divisible by numHeads or adjusting the output dimension to outputFeatures * numHeads.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/NeuralNetworks/Layers/Graph/GraphAttentionLayer.cs (1)

485-504: Consider implementing full backpropagation through attention softmax.

The attention weight gradient computation is simplified and doesn't backpropagate through the softmax normalization (as noted in the comment at line 485). While this enables training to work, it may lead to suboptimal gradient updates for the attention parameters.

For improved training dynamics, consider implementing the full gradient that accounts for the softmax normalization, which would include the Jacobian of the softmax operation. The full gradient for attention coefficient α_ij involves:

• ∂L/∂e_ij (pre-softmax scores)
• The softmax Jacobian: ∂α_ij/∂e_ik = α_ij(δ_jk - α_ik)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/NeuralNetworks/Layers/Graph/GraphSAGELayer.cs (1)

3-18: Doc still mentions LSTM aggregator which is not implemented

The main XML remark still says:

AGGREGATE is an aggregation function (mean, max, LSTM)

but the actual implementation and SAGEAggregatorType only support Mean, MaxPool, and Sum.

To avoid misleading users, please either:

• Add an LSTM option to SAGEAggregatorType and implement it, or
• Update the remark to match the enum and implementation, e.g.:

-/// AGGREGATE is an aggregation function (mean, max, LSTM), and σ is an activation function.
+/// AGGREGATE is an aggregation function (mean, max-pooling, or sum), and σ is an activation function.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/NeuralNetworks/Layers/Graph/EdgeConditionalConvolutionalLayer.cs (1)

198-205: Clarify edge feature tensor shape vs indexing convention.

The XML doc for SetEdgeFeatures says shape [batch, numEdges, edgeFeatureDim], but in Forward you index:

int edgeIdx = i * numNodes + j;
...
_edgeFeaturesTensor[b, edgeIdx, f]

which implicitly assumes numEdges == numNodes * numNodes and a fixed (i,j) → edgeIdx mapping. That’s closer to “[batch, numNodes * numNodes, edgeFeatureDim]” (or even a dense [batch, numNodes, numNodes, edgeFeatureDim] layout).

To avoid confusion for users implementing data loaders, I suggest:

• Updating the XML doc to explicitly describe the expected layout (e.g., “edgeIdx = i * numNodes + j with numEdges = numNodes * numNodes”), or
• Generalizing the implementation to support a more conventional sparse edge list [(src, dst, features)] with a separate edge index tensor.

If you want, I can propose a concrete doc update or an alternative sparse edge layout that still fits the current API.

Also applies to: 231-283

src/NeuralNetworks/Layers/Graph/GraphTransformerLayer.cs (2)

273-296: Clarify and validate adjacency matrix shape vs indexing.

SetAdjacencyMatrix initializes _structuralBias using adjacencyMatrix.Shape[1], while attention masking later indexes _adjacencyMatrix![b, i, j], implying an adjacency of shape [batchSize, numNodes, numNodes]. Other graph layers in this repo often treat adjacency as [numNodes, numNodes], so there is a risk of shape mismatch or out-of-range indexing if callers pass a 2D adjacency.

Consider:

• Documenting the expected adjacency shape in the XML remarks and/or interface.
• Adding runtime validation in Forward to ensure consistency, e.g.:

    public override Tensor<T> Forward(Tensor<T> input)
    {
        if (_adjacencyMatrix == null)
        {
            throw new InvalidOperationException(
                "Adjacency matrix must be set using SetAdjacencyMatrix before calling Forward.");
        }

-        _lastInput = input;
-        int batchSize = input.Shape[0];
-        int numNodes = input.Shape[1];
+        _lastInput = input;
+        int batchSize = input.Shape[0];
+        int numNodes = input.Shape[1];
+
+        // Basic shape checks: adjacency [batchSize, numNodes, numNodes]
+        if (_adjacencyMatrix.Shape.Length != 3 ||
+            _adjacencyMatrix.Shape[0] != batchSize ||
+            _adjacencyMatrix.Shape[1] != numNodes ||
+            _adjacencyMatrix.Shape[2] != numNodes)
+        {
+            throw new InvalidOperationException(
+                "Adjacency matrix must have shape [batchSize, numNodes, numNodes] " +
+                "matching the input tensor.");
+        }

If the project standard is a 2D adjacency ([numNodes, numNodes]), you may instead want to drop the b index in _adjacencyMatrix accesses and adjust _structuralBias sizing accordingly.

Also applies to: 431-435

---

79-86: LayerNorm parameters and dropout are unused; docs over-promise behavior.

The class defines _layerNorm1Scale/_Bias, _layerNorm2Scale/_Bias, and _dropoutRate, and the XML docs claim “skip connections and layer normalization” and “dropout” support. However, Forward never applies layer normalization or dropout.

Either:

• Wire these features into the forward path (preferred), e.g., apply layer norm after residual additions and apply dropout to attention weights using _dropoutRate and _random; or
• Remove the unused fields and adjust XML documentation so it reflects the actual behavior.

This will keep the public API and docs aligned with the implementation.

Also applies to: 88-93, 151-152, 351-365

src/NeuralNetworks/Layers/Graph/MessagePassingLayer.cs (4)

3-29: Public delegates are currently unused and expand the API surface without clear usage

MessageFunction<T>, AggregationFunction<T>, and UpdateFunction<T> are public but not used anywhere in this file. Exposing them publicly commits you to this shape long‑term and may confuse users if there is no way to plug custom functions into MessagePassingLayer<T> yet.

Consider either:

• Wiring these delegates into MessagePassingLayer<T> (e.g., as optional injection points for custom message/aggregation/update logic), or
• Making them internal/private until you actually need them.

---

94-98: Edge weights are allocated but never used in forward/backward/parameter management

_edgeWeights and _edgeWeightsGradient are introduced and (for _edgeWeights) initialized, but they are:

• Not used in Forward (edge features are concatenated directly, without any learned transform).
• Not part of gradient computation in Backward.
• Not included in GetParameters / SetParameters / UpdateParameters.
• Not reset in ResetState.

This makes them effectively dead parameters; they consume memory and complicate the implementation without affecting model behavior or being trainable.

You probably want to either:

1. Remove them for now to keep the implementation minimal and accurate to its capabilities, or
2. Fully wire them in so that edge features are projected via _edgeWeights (with corresponding gradients and parameter management).

Example of option (1) — removing unused edge weights:

-    /// <summary>
-    /// Edge feature transformation weights (optional).
-    /// </summary>
-    private Matrix<T>? _edgeWeights;
-
...
-        // Edge feature transformation
-        if (useEdgeFeatures && edgeFeatureDim > 0)
-        {
-            _edgeWeights = new Matrix<T>(edgeFeatureDim, _messageFeatures);
-        }
...
-        // Initialize edge weights if used
-        if (_edgeWeights != null)
-        {
-            T scaleEdge = NumOps.Sqrt(NumOps.FromDouble(2.0 / (_edgeWeights.Rows + _edgeWeights.Columns)));
-            InitializeMatrix(_edgeWeights, scaleEdge);
-        }

If you keep them, make sure they’re actually used in Forward, have gradients in Backward, and are included in GetParameters / SetParameters / UpdateParameters.

Also applies to: 129-143, 218-223

---

410-470: Reset gate is computed but never used in the GRU‑style update

The comments and structure suggest a GRU‑style update, with both reset and update gates:

• resetGate is computed from _resetWeights, _resetMessageWeights, _resetBias.
• updateGate is computed from _updateWeights, _updateMessageWeights, _updateBias.

However, in the final update:

// Compute new features: h' = (1 - z) * h + z * m
...
output[b, i, f] = NumOps.Add(oldContribution, newContribution);

only updateGate participates; resetGate is never used. As a result:

• _resetWeights, _resetMessageWeights, and _resetBias have no effect on the output.
• The “GRU‑style” description is misleading; this is a single‑gate interpolation between the old features and aggregated messages.

You might want to either:

• Incorporate resetGate into a candidate state (e.g., gate _lastAggregated before combining, or follow the usual GRU formulation), or
• Drop the reset gate entirely (fields, initialization, gradients, and parameter serialization) and document this as a single‑gate update to keep the implementation honest and lean.

---

643-660: ResetState omits _edgeWeightsGradient and relies on never‑used state

ResetState clears cached inputs/outputs and all gradient holders except _edgeWeightsGradient, which remains untouched. Combined with the fact that _edgeWeightsGradient is never assigned in Backward, this is another indication that the edge‑weight path is half‑wired.

Once you either fully wire or remove edge weights (as noted earlier), make sure ResetState is kept in sync with the actual set of trainable parameters and their gradients to avoid stale state.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro